Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert all DID Method entries to JSON files. #353

Merged
merged 113 commits into from Nov 14, 2021
Merged

Convert all DID Method entries to JSON files. #353

merged 113 commits into from Nov 14, 2021

Conversation

msporny
Copy link
Member

@msporny msporny commented Nov 1, 2021

This PR is the first in a series to convert all DID Method entries to JSON files that contain links to information about the DID Method. This PR is purely additive, it only adds files that are not used at the moment, a future PR will use these files to auto-generate the table(s) of DID Methods.

This PR also removes the "PROVISIONAL" status marker, keeping the "DEPRECATED" and "WITHDRAWN" markers.


Preview | Diff

methods/key.json Outdated Show resolved Hide resolved
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tweak for internal consistency. Otherwise, appears to do what it says on the tin.

methods/3.json Outdated
"description": "",
"status": "",
"registry": "Ceramic Network",
"contact": "<a href=\"mailto:oed@3box.io\">Joel Thorstensson</a>",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<script>alert(1);</script>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer json to not include XSS vectors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At present, we allow HTML markup in all entries (this is nothing new, we've operated like this for a while). We have a review process that's supposed to catch bad things like script injection from happening. If something bad does get through, it runs in the browser context anyway -- that is, the security model is the same as browsing to any website.

I think we're good here, at least for the time being, and we can always add a post-processing step that only allows a subset of HTML tags (but I expect that to be pretty heavy handed). Or we require contact information to be an email address (which means that almost every entry is not going to have that information).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At present, we allow HTML markup in all entries (this is nothing new, we've operated like this for a while).

This PR is the first time we have added JSON files which contain html markup to the registry, and we should not start by encouraging dom based xss via respec.

is contact always an email? why not call it "email" and make it an email?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is contact always an email? why not call it "email" and make it an email?

No, it isn't. Sometimes it's an email. Sometimes it's a website. Sometimes it's just a name. Sometimes it's just the name of a company. So, what of those options are we going to force people to do (because they all seem like valid options, IMHO).

Copy link
Contributor

@OR13 OR13 Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok thats fair, I think its easier to express that as optional fields.... Like this:

title: DID Method Registry Entry
description: This schema defines the shape of did method registrations in the did spec registries
type: object
required:
  - status
  - name
  - specification
properties:
  status:
    description: "The status of the entry in the registry".
    type: string
    enum: ["accepted", "withdrawn", "deprecated"]
  name:
    description: https://www.w3.org/TR/did-core/#dfn-did-methods
    type: string
    maxLength: 256
    example: "did:example"
  description:
    description: "A description of the entry in the registry.".
    type: string
    maxLength: 256
    example: "did:example is reserved for examples."
  specification:
    description: "An active URL that resolves to a human readable did method specification".
    type: string
    maxLength: 1024
    example: "https://example.com/example-did-method-spec"
  contactName:
    description: "An optional contact point name".
    type: string
    maxLength: 256
    example: "Example Corp. Support"
  contactEmail:
    description: "An optional contact point email".
    type: string
    maxLength: 256
    example: "support@example.com"
  contactWebsite:
    description: "An optional contact point website".
    type: string
    maxLength: 1024
    example: "example.com"
  verifiableDataRegistry:
    description: https://www.w3.org/TR/did-core/#dfn-verifiable-data-registry
    type: string
    maxLength: 256
    example: "Bitcoin / Ethereum + IPFS / Hyperledger"
  rubricEvaluation:
    description: "An active URL that resolves to a human readable did method evaluation".
    type: string
    maxLength: 1024
    example: ""
additionalProperties: false

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

additionalProperties: false <- this is key.

Copy link
Member Author

@msporny msporny Nov 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good, thanks for being specific.

I'm reworking the schema you have above into the conversion script (I don't think we need maxLength, but shrug -- if you want it go ahead and put it in there). Bonus points if you want to work the JSON Schema above into a Github merge test for all PRs in the methods/ directory.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I implemented the schema above in the latest PR commits. The only change I didn't take was the "accepted" language... I chose to use "registered" instead.

We may not even want to have "registered", since it's obvious that it's registered if it's in the DID Spec Registries. So, it's either blank or "withdrawn"/"deprecated". We may even want to collapse both of those entries into "deprecated".

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@msporny thanks for this PR.

I have a few concerns, listing them in priority order here:

1. Preventing Double Work.

If we accept this PR, it should be complete with instructions for new registrations, and functional, in that there is only a single entry related to existing registry entries in the repo after merging.

Otherwise, we will have folks adding to the "old registry" making more work for maintainers of the "new registry"...

2. Documenting All JSON Members

There should be a description of each JSON member, and its requirements.

Allowed characters, min max length, required or optional, etc...

3. Document New Registration Process

Instructions should be provided so we can adopt the new process completely, and close all open PRs related to the old process.

4. Can we do this via code first?

is there a script that is used to generate these files?

I prefer to review a script first, not a bunch of generated files... easier to adjust the output as desired.

This would make it much easier to review the intention behind the PR.

Next Steps

I can help document the proposed JSON members and help on a script to generate these files.

Can we remove the new files being added and instead add the script, json requirements and new registration requirements, and then generate the data at the end, once we have agreed to the new structure?

@OR13
Copy link
Contributor

OR13 commented Nov 1, 2021

Here are some constraints on the registry members and their descriptions: #353 (comment)

@msporny
Copy link
Member Author

msporny commented Nov 1, 2021

  1. Can we do this via code first?

Code is here:

https://gist.github.com/msporny/1fdcb0b98050b849614e1c9fd6a341de

  1. Preventing Double Work.
    If we accept this PR, it should be complete with instructions for new registrations, and functional, in that there is only a single entry related to existing registry entries in the repo after merging.
    Otherwise, we will have folks adding to the "old registry" making more work for maintainers of the "new registry"...

No, for two reasons:

  1. The PR is meant to be coupled with other PRs for instructions for new registrations (feel free to work on that PR, please).
  2. There is a conversion script, so anything merged in the interim is easy to convert out of the main index.html until we replace it with the ReSpec autogenerator (which I plan to get done soon-ish), so I expect that this won't be a huge problem in practice.

There is a plan to do what you want, but -1 to bundling them all into a single PR. All this PR does is the conversion. A separate PR will do the instructions for new registrations. Yet another separate PR will do the ReSpec auto-generation of the DID Method table. That's the plan so that we can have PRs that do one thing so it's easier to collaborate and merge on each subunit of work.

  1. Documenting All JSON Members
    There should be a description of each JSON member, and its requirements.

Please document this and put it into a PR in a way that is acceptable to you so that we can review.

  1. Document New Registration Process
    Instructions should be provided so we can adopt the new process completely, and close all open PRs related to the old process.

Same as above... separate PR, please.

If you choose to work on the two separate PRs above, please let me know so we don't duplicate work.

@msporny msporny force-pushed the msporny-did-json branch 3 times, most recently from a73f65f to 1fd95c2 Compare November 1, 2021 19:59
@OR13
Copy link
Contributor

OR13 commented Nov 1, 2021

@msporny based on your feedback, let me take a stab at a separate PR, consider the ball in my court.

@OR13
Copy link
Contributor

OR13 commented Nov 2, 2021

Lets close this PR, and continue to discussion on: #356

This PR adds a too many generated files, and does not meet the acceptance criteria for a transition of registry format I listed above.

@msporny msporny marked this pull request as draft November 4, 2021 15:52
@OR13
Copy link
Contributor

OR13 commented Nov 4, 2021

@msporny can we close this? your PR #357 seems a better starting place.

@msporny
Copy link
Member Author

msporny commented Nov 4, 2021

@msporny can we close this? your PR #357 seems a better starting place.

PR #357 is based on top of this one. :)

What we can do here is eliminate all the new fields? I think that's what you were asking for on the call today? I'm pretty sure I heard @jricher say that he hated the description field with the burning passion of a thousand suns, so I could eliminate that one as well as the others... but we might want to keep them around to see how much people like/hate them.

I'm fine w/ clawing this entire PR back to "just the information we have today".

@mprorock
Copy link
Contributor

mprorock commented Nov 4, 2021

I would like to also see a an optional link to an example in addition to the optional link to to the rubric eval in the schema for the json entries

@msporny msporny force-pushed the msporny-did-json branch 2 times, most recently from 92666fb to 3598609 Compare November 6, 2021 18:19
@msporny
Copy link
Member Author

msporny commented Nov 6, 2021

I would like to also see a an optional link to an example in addition to the optional link to to the rubric eval in the schema for the json entries

Can we do this in another PR that just focuses on "fields we want to add"?

@msporny
Copy link
Member Author

msporny commented Nov 6, 2021

Based on feedback from the last telecon, I have:

  • Updated this PR to only cover converting the current HTML-based entry method that we have to a JSON-based entry method.
  • Removed any new proposed fields, which will be handled in a future PR (that proposes the new field(s) and modifies all JSON files to include the new field(s)).
  • Temporarily included the tooling to do the conversion, which will be removed before merging to main.
  • Renamed "provisional" to "registered".

I believe that removes every controversy in this PR that was identified during the last telecon. I believe this PR is now ready for review and merge.

@msporny msporny marked this pull request as ready for review November 6, 2021 18:25
@msporny msporny requested a review from OR13 November 6, 2021 18:25
@msporny
Copy link
Member Author

msporny commented Nov 14, 2021

Editorial, multiple reviews, changes requested and made, no remaining objections, merging.

@msporny msporny merged commit f1e3187 into main Nov 14, 2021
@msporny msporny deleted the msporny-did-json branch November 14, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants